-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wapm.io integration #43
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this. This is a great idea. We need this.
Let's think about feature design a little bit!
I know of two WASM package manager thingies:
WAPM is real. WARG is a proposal with a reference implementation that's under active development that might become real. If we generalize, we should support both registries. This leads us to a cli invocation like this:
serval pull wapm author/package@version
serval pull wapm ceejbot/loudbot
serval pull warg bytecodealliance.org/[email protected]
Note that I'm going with @version
optionally because that's what all the other package manager clis seem to do.
We can pass the registry string into the pull()
function in the cli implementation, and then it can be parsed by the specific registry implementation.
I have some specific suggestions about how we handle the specific registry implementations. Right now you're passing a string into a parse()
function, which goes on to fill out some struct fields with registry-specific format strings at runtime. I suggest we restructure to make Rust's type system do this work for us.
Here's a possible implementation:
enum PackageRegistry {
Wapm,
Warg
}
impl PackageRegistry {
fn download_url(&self, pkg: &PackageSpec) -> String {
match self {
PackageRegistry::Wapm => {
format!("https://registry-cdn.wapm.io/contents/{}/{}/{}/target/wasm32-wasi/release/{}.wasm", pkg.author, pkg.name, pkg.version, pkg.name)
},
PackageRegistry::Warg => todo!(),
}
}
// even cooler....
fn download(&self, pkg: &PackageSpec) -> Result<Bytes, ServalError> {
// do the work of downloading from this kind of registry
}
}
pub struct PackageSpec {
author: String,
name: String,
version: String,
registry: PackageRegistry,
// any other useful fields
}
impl PackageSpec {
// other useful functions here
pub fn download_url(&self) -> String {
self.registry.download_url(self)
}
}
impl TryFrom<&str> for PackageSpec {
// put your parsing code here
}
The advantage of this approach is that different registries might have different ways to download, e.g., different ways to authenticate. We can hide that inside a specific registry download()
implementation if we need to and get rid of the download_url()
function. You can probably think of a bunch of different ways to achieve the goal of hiding the registry-specific stuff as much as possible.
The interesting Rust lesson here is to notice that I swapped out your match on a string approach for matching on an enum.
YAY for thinking of this feature, btw. Smrt. 🏆 |
Thanks, these are awesome suggestions. I'm so not used to having an "implementation" for an Enum - I initially had a similar setup to yours with a Registry enum (but different / less idiomatic implementation downstream from it) but then discarded it because I got stuck somewhere. Definitely rewriting this based on your suggestion! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As before, some optional comments aimed at being rusty!
Pull { | ||
/// The name of the software package, formatted as | ||
/// [[protocol://]registry.tld/]author/packagename[@version][:module] | ||
identifer: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can eventually make the identifier a type more specific than a string and have clap parse it for us. Some examples here. There is no need to do that in this PR, however.
cli/src/main.rs
Outdated
@@ -289,6 +295,100 @@ fn blocking_maybe_discover_service_url( | |||
Ok(format!("http://{addr}:{port}")) | |||
} | |||
|
|||
/// Pull a Wasm package from a package manager, generate its manifest, and store it. | |||
fn pull(identifer: String) -> Result<()> { | |||
let pkg_spec = PackageSpec::try_from(identifer).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will teach you a new trick with Rust! Or maybe you already know this one, but here is let-else:
let pkg_spec = Ok(PackageSpec::try_from(identifer)) else {
// tell the user what they got wrong
// then exit with a non-zero code
}
This is nicer than panicking here if the format is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhhh... nice. I was theoretically aware of let else but did not grok the specific usage. Will hunt down a bunch of unwrap()
s and clean them up.
cli/src/main.rs
Outdated
/// Pull a Wasm package from a package manager, generate its manifest, and store it. | ||
fn pull(identifer: String) -> Result<()> { | ||
let pkg_spec = PackageSpec::try_from(identifer).unwrap(); | ||
log::debug!("{:#?}", pkg_spec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is absolutely zero reason to change this, but I note that the dbg!() macro is another option for this kind of fast printf debugging.
PackageRegistry::Warg => todo!(), | ||
} | ||
} | ||
// even cooler.... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey wait I recognize that ellipsis with an extra dot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blatant plagiarism. painstakingly embellished for hours and hours, though, I claim fair use 😹
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posting a comment solely to get GitHub to stop showing me this PR under my Review Requests 😂 Hope you're well!
Since I was planning on looking into executing arbitrary existing software packages via Serval Mesh, I had the sudden urge of making wapm.io integration happen. My goal for this change is to make it as easy as
Where
serval pull
wouldserval store
on the manifest.First actual code submission, please be gentle 😸
This is a very rough work in progress - just wanted to share it already because my Rust is, well, rusty, and you might have conceptual opinions as well...
Please let me know what you think!